-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make handling of EMME matrices less error-prone and more flexible #525
Conversation
Also add docstrings and remove deprecated imports
@hsl-petrhaj @johpiip are you able to look at this? |
I did not have time for testing it yet, I should be able to check it this week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing looks fine with mock run, so I approve it for now.
Now testing still with Emme run. |
@zptro The validate script (helmet_validate_inputfiles.py) now does not work, as the parameter names have changed. Please also check the tests.
|
For some reason I had to increase the size of the Emmebank for extra attributes, I guess we have some new ones now? I will then need to update helmet-docs as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fine now. Not sure why I got the "not enough space" error in Emme, but perhaps this could be coming from somewhere else. Anyway it was easy to get this version to run.
That is not very good. I found the problem and fixed. We do not need an extra attribute for walking, as we do not assign walk trips. |
I am getting some division by zero errors when running with Emme in the second iteration. Trying to figure out if it is related to those changes. |
@zptro
Could there be some issue with the demand matrix? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still needs some further checks.
The problem was that transit time matrices were 9999, because assignment did not write anything to them, because I had accidentally dropped that line. Now this is fixed. |
@johpiip could you review this now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi and thank you for your PR! I am sad to see the Emme matrix cheatsheet go but I understand the reasoning behind. 😊
I tried running this branch with the samll test network. I encountered an type error, maybe you can check it?
[UI-event] Initializing run of scenarios: testi_emme-matrices
[INFO 10:21] Checking base inputdata...
[INFO 10:21] Checking base zonedata & scenario-based input data...
[INFO 10:21] Checking input data for scenario #0 ...
[INFO 10:21] Successfully validated all input files
[UI-event] Running scenario "testi_emme-matrices"
[INFO 10:21] Initializing Emme...
[INFO 10:21] Starting Emme...
[INFO 10:21] Emme started
[INFO 10:21] Initializing matrices and models...
[INFO 10:21] Starting simulation with 15 iterations...
[INFO 10:21] Calculates road charges for time period aht...
[INFO 10:21] Calculates road charges for time period pt...
[INFO 10:21] Calculates road charges for time period iht...
[INFO 10:21] Pedestrian assignment started...
[INFO 10:21] Pedestrian assignment performed for scenario 51
[INFO 10:21] Sets bike functions for scenario 51
[INFO 10:21] Bike assignment started...
[INFO 10:21] Bike assignment performed for scenario 51
[INFO 10:21] Sets car and transit functions for scenario 51
[INFO 10:21] Car assignment started...
[INFO 10:21] Car assignment performed for scenario 51
[INFO 10:21] Stopping criteria: RELATIVE_GAP, iteration 2 / 200
[INFO 10:21] Calculates cumulative travel times for scenario 51
[INFO 10:21] Transit assignment started...
[INFO 10:21] Transit assignment performed for scenario 51
[EXCEPTION] Traceback (most recent call last):
[EXCEPTION] File "C:\Users\PiippJo\Downloads\helmet-model-system-refactor-emme_matrices\Scripts\helmet.py", line 269, in <module>
[EXCEPTION] main(args)
[EXCEPTION] File "C:\Users\PiippJo\Downloads\helmet-model-system-refactor-emme_matrices\Scripts\helmet.py", line 96, in main
[EXCEPTION] args.use_fixed_transit_cost, iterations==0)
[EXCEPTION] File "C:\Users\PiippJo\Downloads\helmet-model-system-refactor-emme_matrices\Scripts\modelsystem.py", line 179, in assign_base_demand
[EXCEPTION] self.ass_model.init_assign(base_demand)
[EXCEPTION] File "C:\Users\PiippJo\Downloads\helmet-model-system-refactor-emme_matrices\Scripts\assignment\emme_assignment.py", line 104, in init_assign
[EXCEPTION] self._copy_matrix("time", "bike", ap0, ap)
[EXCEPTION] File "C:\Users\PiippJo\Downloads\helmet-model-system-refactor-emme_matrices\Scripts\assignment\emme_assignment.py", line 277, in _copy_matrix
[EXCEPTION] "{} {}".format(to_mtx["description"], ass_period_2.name))
[EXCEPTION] TypeError: string indices must be integers
[ERROR] [...] TypeError: string indices must be integers
[UI-event] Scenario "testi_emme-matrices" finished
My setting were following:
2023-08-17 10:21:41 [DEBUG] helmet_version=v4.1.3
2023-08-17 10:21:41 [DEBUG] sys.version_info=3
2023-08-17 10:21:41 [DEBUG] sys.path=['C:\\Users\\PiippJo\\Downloads\\helmet-model-system-refactor-emme_matrices\\Scripts', 'C:\\Program Files\\INRO\\Emme\\Emme 4\\Emme-23.00.01.23\\Python37\\python37.zip', 'C:\\Program Files\\INRO\\Emme\\Emme 4\\Emme-23.00.01.23\\Python37\\DLLs', 'C:\\Program Files\\INRO\\Emme\\Emme 4\\Emme-23.00.01.23\\Python37\\lib', 'C:\\Program Files\\INRO\\Emme\\Emme 4\\Emme-23.00.01.23\\Python37', 'C:\\Users\\PiippJo\\AppData\\Roaming\\Python\\Python37\\site-packages', 'C:\\Program Files\\INRO\\Emme\\Emme 4\\Emme-23.00.01.23\\Python37\\lib\\site-packages', 'C:\\Program Files\\INRO\\Emme\\Emme 4\\Emme-23.00.01.23\\Python37\\lib\\site-packages\\win32', 'C:\\Program Files\\INRO\\Emme\\Emme 4\\Emme-23.00.01.23\\Python37\\lib\\site-packages\\win32\\lib', 'C:\\Program Files\\INRO\\Emme\\Emme 4\\Emme-23.00.01.23\\Python37\\lib\\site-packages\\Pythonwin']
2023-08-17 10:21:41 [DEBUG] log_level=DEBUG
2023-08-17 10:21:41 [DEBUG] log_format=JSON
2023-08-17 10:21:41 [DEBUG] end_assignment_only=False
2023-08-17 10:21:41 [DEBUG] is_agent_model=False
2023-08-17 10:21:41 [DEBUG] do_not_use_emme=False
2023-08-17 10:21:41 [DEBUG] separate_emme_scenarios=False
2023-08-17 10:21:41 [DEBUG] save_matrices=True
2023-08-17 10:21:41 [DEBUG] del_strat_files=True
2023-08-17 10:21:41 [DEBUG] scenario_name=testi_emme-matrices
2023-08-17 10:21:41 [DEBUG] results_path=E:\Results\Jopi_testi
2023-08-17 10:21:41 [DEBUG] emme_path=D:\helmet_41\Projektit\pienitestiprojekti\pienitestiprojekti.emp
2023-08-17 10:21:41 [DEBUG] first_scenario_id=51
2023-08-17 10:21:41 [DEBUG] first_matrix_id=400
2023-08-17 10:21:41 [DEBUG] baseline_data_path=C:\Users\PiippJo\Downloads\helmet-model-system-olusanya\Scripts\tests\test_data\Base_input_data
2023-08-17 10:21:41 [DEBUG] forecast_data_path=C:\Users\PiippJo\Downloads\helmet-model-system-olusanya\Scripts\tests\test_data\Scenario_input_data\2030_test
2023-08-17 10:21:41 [DEBUG] iterations=15
2023-08-17 10:21:41 [DEBUG] max_gap=1.0
2023-08-17 10:21:41 [DEBUG] rel_gap=0.01
2023-08-17 10:21:41 [DEBUG] use_fixed_transit_cost=False
Thank you both for testing this thoroughly (i.e., doing the job that I should have done). ❤️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test run succeeded and old and new matrices have same names, descriptions, and order. Nice work! 😊 Sorry for the delay in reviewing!
Hey, sorry, I still have a question! By mistake, I ran the second model run with wrong scripts so even though everything work now, matrices are in different order and descriptions are different. Other matrices are in same order are previously but trip_part matrices are differently. Does this clarify: Is this preferred behaviour or should we have them in the same order? Descriptions in this PR are more condensed than previously and do not add much new information on top of the matrix name. Did you consider having the same description info as earlier? Examples can be seen in the image above, too. |
Good comments! The so-called trip-part matrix dictionary now goes directly into the specification of matrix results calculation (which means they are split into total matrices and matrices by mode subset), so that is the reason why it cannot be in exactly the same order as before. But I changed the order a bit so that it is as close as possible to what it was before. I also changed the description to a more descriptive one, which is in fact also what it is in the Modeller tool specification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works on my computer. Tiny (about 1 ppm) differences in the results compared to the latest olusanya, not a problem by my standards.
The demand matrix looks the same, amount of trips is the same as well. I did not check the order of the matrices.
With this PR, EMME matrix ids are no longer set manually. Instead, they are created with an algorithm according to the same principles as before:
TransitSpecification
; instead the appropriate matrix names are taken from the parameter file.The matrix id numbers are now internally stored in a more simple dict structure (which directly corresponds to how the matrices are handled in the scripts), which is created in a separate method:
helmet-model-system/Scripts/assignment/emme_assignment.py
Lines 337 to 344 in d8fc46b